Add OneDrive, Google Drive, and Dropbox storage providers #111
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public Task<DriveItem> UploadAsync(string driveId, string path, Stream content, string? contentType, CancellationToken cancellationToken) | ||
| { | ||
| throw new NotSupportedException("Graph upload requires a configured OneDrive runtime environment."); | ||
| } | ||
|
|
There was a problem hiding this comment.
GraphOneDriveClient is selected whenever OneDriveStorage receives a GraphServiceClient, but its core methods are stubs (upload/download/delete immediately throw NotSupported) while exists/metadata/list return defaults, so any runtime configured with a real Graph client will fail all storage calls despite CreateContainer succeeding. The new OneDrive provider is therefore unusable with the intended client until these operations are implemented.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds three new cloud storage providers (OneDrive, Google Drive, and Dropbox) to the ManagedCode.Storage library, along with comprehensive fake-client tests and a dotnet-install.sh script for local SDK setup. The implementation follows the existing storage abstraction patterns but contains several critical and moderate issues that need to be addressed.
Key Changes:
- New storage provider implementations for OneDrive, GoogleDrive, and Dropbox with full CRUD operations
- Client wrapper abstractions (IOneDriveClient, IGoogleDriveClient, IDropboxClientWrapper) enabling testability
- Comprehensive test suite using in-memory fake clients to validate round-trip operations
- Integration plan documents for each provider outlining implementation status and future work
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet-install.sh | Standard .NET installation script (1888 lines) for CI/CD SDK provisioning |
| Tests/.../CloudDriveStorageTests.cs | Test suite with fake client implementations for all three providers |
| Tests/.../ManagedCode.Storage.Tests.csproj | Added project references for the three new storage providers |
| Storages/.../OneDrive/* | OneDrive storage implementation with Graph SDK integration and provider registration |
| Storages/.../GoogleDrive/* | Google Drive storage with DriveService client wrapper and folder hierarchy support |
| Storages/.../Dropbox/* | Dropbox storage implementation with API wrapper and path normalization |
| ManagedCode.Storage.slnx | Solution updated to include the three new storage project references |
Critical Issues Identified:
- Dropbox MimeType field incorrectly assigned to file name instead of content type
- GraphOneDriveClient throws NotSupportedException, making it non-functional
- GoogleDrive BuildFullPath ignores RootFolderId, causing incorrect file placement
- Multiple missing cancellationToken usages in Dropbox client operations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public async Task<bool> DeleteAsync(string rootPath, string path, CancellationToken cancellationToken) | ||
| { | ||
| var fullPath = Combine(rootPath, path); | ||
| await _client.Files.DeleteV2Async(fullPath); |
There was a problem hiding this comment.
The cancellationToken parameter is not passed to the DeleteV2Async call, which means delete operations cannot be cancelled. Pass the cancellationToken to the API call.
| await _client.Files.DeleteV2Async(fullPath); | |
| await _client.Files.DeleteV2Async(fullPath, cancellationToken: cancellationToken); |
| public async Task<bool> ExistsAsync(string rootPath, string path, CancellationToken cancellationToken) | ||
| { | ||
| var fullPath = Combine(rootPath, path); | ||
| try | ||
| { | ||
| await _client.Files.GetMetadataAsync(fullPath); | ||
| return true; | ||
| } | ||
| catch (ApiException<GetMetadataError> ex) when (ex.ErrorResponse.IsPath && ex.ErrorResponse.AsPath.Value.IsNotFound) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| public async Task<DropboxItemMetadata?> GetMetadataAsync(string rootPath, string path, CancellationToken cancellationToken) | ||
| { | ||
| var fullPath = Combine(rootPath, path); | ||
| try | ||
| { | ||
| var metadata = await _client.Files.GetMetadataAsync(fullPath); | ||
| return metadata.IsFile ? ToItem(metadata.AsFile) : null; | ||
| } | ||
| catch (ApiException<GetMetadataError> ex) when (ex.ErrorResponse.IsPath && ex.ErrorResponse.AsPath.Value.IsNotFound) | ||
| { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The GetMetadataAsync and ExistsAsync methods don't pass the cancellationToken to the API calls, preventing cancellation of these operations.
| MimeType = file.Name | ||
| }; | ||
| } |
There was a problem hiding this comment.
The MimeType field is incorrectly assigned to the file name instead of a proper MIME type value. This will cause incorrect content type information to be stored in the metadata.
| MimeType = file.Name | |
| }; | |
| } | |
| MimeType = GetMimeType(file.Name) | |
| }; | |
| } | |
| // Helper method to get MIME type from file name/extension | |
| private static string GetMimeType(string fileName) | |
| { | |
| var extension = Path.GetExtension(fileName).ToLowerInvariant(); | |
| switch (extension) | |
| { | |
| case ".txt": return "text/plain"; | |
| case ".htm": | |
| case ".html": return "text/html"; | |
| case ".css": return "text/css"; | |
| case ".csv": return "text/csv"; | |
| case ".jpg": | |
| case ".jpeg": return "image/jpeg"; | |
| case ".png": return "image/png"; | |
| case ".gif": return "image/gif"; | |
| case ".bmp": return "image/bmp"; | |
| case ".webp": return "image/webp"; | |
| case ".svg": return "image/svg+xml"; | |
| case ".json": return "application/json"; | |
| case ".xml": return "application/xml"; | |
| case ".pdf": return "application/pdf"; | |
| case ".zip": return "application/zip"; | |
| case ".tar": return "application/x-tar"; | |
| case ".gz": return "application/gzip"; | |
| case ".mp3": return "audio/mpeg"; | |
| case ".mp4": return "video/mp4"; | |
| case ".avi": return "video/x-msvideo"; | |
| case ".mov": return "video/quicktime"; | |
| case ".doc": return "application/msword"; | |
| case ".docx": return "application/vnd.openxmlformats-officedocument.wordprocessingml.document"; | |
| case ".xls": return "application/vnd.ms-excel"; | |
| case ".xlsx": return "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"; | |
| case ".ppt": return "application/vnd.ms-powerpoint"; | |
| case ".pptx": return "application/vnd.openxmlformats-officedocument.presentationml.presentation"; | |
| default: return "application/octet-stream"; | |
| } | |
| } |
| public async Task EnsureRootAsync(string rootPath, bool createIfNotExists, CancellationToken cancellationToken) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(rootPath)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var normalized = Normalize(rootPath); | ||
| try | ||
| { | ||
| await _client.Files.GetMetadataAsync(normalized); | ||
| } | ||
| catch (ApiException<GetMetadataError> ex) when (ex.ErrorResponse.IsPath && ex.ErrorResponse.AsPath.Value.IsNotFound) | ||
| { | ||
| if (!createIfNotExists) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| await _client.Files.CreateFolderV2Async(normalized, autorename: false); | ||
| } | ||
| } |
There was a problem hiding this comment.
The EnsureRootAsync implementation creates a folder but doesn't verify if it already exists before creation, which could lead to exceptions if createIfNotExists is false but the folder doesn't exist. The method should check existence first and only create when both the folder is missing and createIfNotExists is true.
|
|
||
| var request = _driveService.Files.Create(fileMetadata, content, contentType ?? "application/octet-stream"); | ||
| request.Fields = "id,name,parents,createdTime,modifiedTime,md5Checksum,size"; | ||
| return await request.UploadAsync(cancellationToken).ContinueWith(async _ => await _driveService.Files.Get(request.ResponseBody.Id).ExecuteAsync(cancellationToken)).Unwrap(); |
There was a problem hiding this comment.
The UploadAsync method uploads the file but then makes a second API call to fetch metadata. The initial upload already returns a DriveFile with the ResponseBody, but instead of using that directly, the code calls Get again. This could be simplified by setting the Fields parameter and using the ResponseBody directly, reducing API calls and improving performance.
| return await request.UploadAsync(cancellationToken).ContinueWith(async _ => await _driveService.Files.Get(request.ResponseBody.Id).ExecuteAsync(cancellationToken)).Unwrap(); | |
| await request.UploadAsync(cancellationToken); | |
| return request.ResponseBody; |
| } | ||
| } | ||
|
|
||
| await Task.CompletedTask; |
There was a problem hiding this comment.
The await Task.CompletedTask statement at line 321 is unnecessary and serves no purpose. The method is already async and yields values, so this line should be removed.
| await Task.CompletedTask; |
| private string BuildFullPath(string? relativePath) | ||
| { | ||
| var normalized = NormalizeRelativePath(relativePath ?? string.Empty); | ||
| return normalized; |
There was a problem hiding this comment.
The BuildFullPath method returns just the normalized path without considering the RootFolderId. Unlike OneDrive and Dropbox implementations which properly combine root and relative paths, this implementation ignores the root folder entirely, which could lead to incorrect file placement.
| return normalized; | |
| var root = NormalizeRelativePath(StorageOptions.RootFolderId ?? string.Empty); | |
| return string.IsNullOrEmpty(root) | |
| ? normalized | |
| : string.IsNullOrEmpty(normalized) | |
| ? root | |
| : $"{root}/{normalized}"; |
| foreach (var item in list.Entries) | ||
| { | ||
| if (item.IsFile) | ||
| { | ||
| yield return ToItem(item.AsFile); | ||
| } | ||
| } | ||
|
|
||
| while (list.HasMore) | ||
| { | ||
| list = await _client.Files.ListFolderContinueAsync(list.Cursor); | ||
| foreach (var item in list.Entries) | ||
| { | ||
| if (item.IsFile) | ||
| { | ||
| yield return ToItem(item.AsFile); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var item in list.Entries) | |
| { | |
| if (item.IsFile) | |
| { | |
| yield return ToItem(item.AsFile); | |
| } | |
| } | |
| while (list.HasMore) | |
| { | |
| list = await _client.Files.ListFolderContinueAsync(list.Cursor); | |
| foreach (var item in list.Entries) | |
| { | |
| if (item.IsFile) | |
| { | |
| yield return ToItem(item.AsFile); | |
| } | |
| foreach (var item in list.Entries.Where(item => item.IsFile)) | |
| { | |
| yield return ToItem(item.AsFile); | |
| } | |
| while (list.HasMore) | |
| { | |
| list = await _client.Files.ListFolderContinueAsync(list.Cursor); | |
| foreach (var item in list.Entries.Where(item => item.IsFile)) | |
| { | |
| yield return ToItem(item.AsFile); |
| foreach (var item in list.Entries) | ||
| { | ||
| if (item.IsFile) | ||
| { | ||
| yield return ToItem(item.AsFile); | ||
| } | ||
| } | ||
|
|
||
| while (list.HasMore) | ||
| { | ||
| list = await _client.Files.ListFolderContinueAsync(list.Cursor); | ||
| foreach (var item in list.Entries) | ||
| { | ||
| if (item.IsFile) | ||
| { | ||
| yield return ToItem(item.AsFile); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var item in list.Entries) | |
| { | |
| if (item.IsFile) | |
| { | |
| yield return ToItem(item.AsFile); | |
| } | |
| } | |
| while (list.HasMore) | |
| { | |
| list = await _client.Files.ListFolderContinueAsync(list.Cursor); | |
| foreach (var item in list.Entries) | |
| { | |
| if (item.IsFile) | |
| { | |
| yield return ToItem(item.AsFile); | |
| } | |
| foreach (var item in list.Entries.Where(item => item.IsFile)) | |
| { | |
| yield return ToItem(item.AsFile); | |
| } | |
| while (list.HasMore) | |
| { | |
| list = await _client.Files.ListFolderContinueAsync(list.Cursor); | |
| foreach (var item in list.Entries.Where(item => item.IsFile)) | |
| { | |
| yield return ToItem(item.AsFile); |
| for link_index in "${!download_links[@]}" | ||
| do | ||
| download_link="${download_links[$link_index]}" | ||
| specific_version="${specific_versions[$link_index]}" | ||
| effective_version="${effective_versions[$link_index]}" | ||
| link_type="${link_types[$link_index]}" | ||
|
|
||
| say "Attempting to download using $link_type link $download_link" | ||
|
|
||
| # The download function will set variables $http_code and $download_error_msg in case of failure. | ||
| download_failed=false | ||
| download "$download_link" "$zip_path" 2>&1 || download_failed=true | ||
|
|
||
| if [ "$download_failed" = true ]; then | ||
| case ${http_code-} in | ||
| 404) | ||
| say "The resource at $link_type link '$download_link' is not available." | ||
| ;; | ||
| *) | ||
| say "Failed to download $link_type link '$download_link': ${http_code-} ${download_error_msg-}" | ||
| ;; | ||
| esac | ||
| rm -f "$zip_path" 2>&1 && say_verbose "Temporary archive file $zip_path was removed" | ||
| else | ||
| download_completed=true | ||
| break | ||
| fi | ||
| done | ||
|
|
||
| if [[ "$download_completed" == false ]]; then | ||
| say_err "Could not find \`$asset_name\` with version = $specific_version" | ||
| say_err "Refer to: https://aka.ms/dotnet-os-lifecycle for information on .NET Core support" | ||
| return 1 | ||
| fi | ||
|
|
||
| remote_file_size="$(get_remote_file_size "$download_link")" | ||
|
|
||
| say "Extracting archive from $download_link" | ||
| extract_dotnet_package "$zip_path" "$install_root" "$remote_file_size" || return 1 | ||
|
|
There was a problem hiding this comment.
This installer downloads and extracts .NET archives from remote feeds using curl/wget without any cryptographic integrity verification (only an optional file-size comparison), which makes the installation process susceptible to supply-chain tampering if the feed or network is compromised. An attacker who can control or intercept the download endpoint (e.g., via a compromised feed, DNS hijack, or TLS interception) could serve a malicious tarball that would be extracted and used as the SDK/runtime with no hash or signature check. To harden this, fetch and validate a known-good checksum or signature for the archive before extraction, and fail the installation if the verification does not match.
|
merged as part of #112 |
KSemenenko
deleted the
codex/implement-cloud-storage-apis-for-.net-10
branch
Summary
Testing
Codex Task